-
Notifications
You must be signed in to change notification settings - Fork 2k
Bitfinex WebSockets API + Fix postonly not triggering re-buy #458
Conversation
Check for balance error also on checkOrder (e.g. bitfinex, using websockets, reports the error later than other exchanges)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, will wait for more eyes from i.e. @nedievas
I'd like more time to read it. Thanks. |
After few tests in the morning, I found balances not working as it should: asset and currency on hold have 0 values. I'll try to test more. |
@@ -282,6 +282,7 @@ module.exports = function container (get, set, clear) { | |||
if (err) return cb(err) | |||
s.api_order = api_order | |||
order.status = api_order.status | |||
if (api_order.reject_reason) order.reject_reason = api_order.reject_reason |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need of this line. Let's keep to overall code structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is in line with lib/engine.js#284. It is more consistent and should be kept (see below).
return cancelOrder(true) | ||
return cb(null, null) | ||
} | ||
if (order.status === 'rejected' && order.reject_reason === 'balance') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we removed previous line 285, we need to correct this to if (order.status === 'rejected' & (order.reject_reason === 'balance' || api_order.reject_reason === 'balance')) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it better to leave in one line then to scarce around the whole code ;) Especially when no body cares to comment every function.
} | ||
if (order.status === 'rejected' && order.reject_reason === 'balance') { | ||
msg('not enough balance for ' + type + ', aborting') | ||
return cb(null, null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No good. Should be return cb(null, false)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain, everywhere it's cb(null, null)
, except for engine.js#231
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should test it. If we leave cb(null, null)
, and we have not enough balance to execute order, we would like it to finish and not to continuously try to execute it. cd(null, false)
cancels further order execution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But on the new order execution the balance is adjusted - and that's what we want?
'oc', | ||
null, | ||
{ | ||
id: order.bitfinex_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
order.order_id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
order.bitfinex_id
is correct (see below).
|
||
var order = { | ||
id: cid, | ||
bitfinex_id: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bitfinex_id: cid
if necessary at all. See line 324.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're using two different IDs here:
- Since we're not using the REST API, we don't immediately get bitfinex's order ID when we submit an order over websockets. Therefore, we're creating our own internal ID (exchange.js#335) and use that within zenbot.
- As soon as we know about bitfinex's ID, we save it in the order object (exchange.js#148) - but zenbot still referns to the order by it's internal ID (
cid
). We can then use thebitfinex_id
to e.g. cancel the order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we are using WS API, we immediately get order ID via 'os' or 'hos'.
I also exchanged bitfinex_id
with opts.order_id
and it canceled the order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we do not know the ID in the moment we send()
the order. How can we be 100% certain the next websockets response is actually the response of the zenbot order we just sent out and not e.g. a manual trade? Therefore I strongly suggest working with internal IDs and storing the bitfinex_id
in the order object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may agree on your position as I had the same problem to instantly get order ID via WS.
balance.currency_hold = ws_balance[opts.currency].available ? n(ws_balance[opts.currency].balance).subtract(ws_balance[opts.currency].available).format('0.00000000') : n(0).format('0.00000000') | ||
balance.asset_hold = ws_balance[opts.asset].available ? n(ws_balance[opts.asset].balance).subtract(ws_balance[opts.asset].available).format('0.00000000') : n(0).format('0.00000000') | ||
|
||
cb(null, balance) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not working. I found a way to take correct values for asset_hold
and currency_hold
from REST1, but if we use the same APIkeys - we get Nonce error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide an example, I don't understand the problem.
According to the bitfinex docs, key 4 of the array is
Amount not tied up in active orders, positions or funding (null if the value is not fresh enough).
Are we on the same page of the definition of currency_hold
and asset_hold
? As far as I understood it, those values are about currency/assets tied up in open/uncanceled/not yet fully returned orders, which we can't use for new orders.
Therefore I think an amount of 0
is correct in most cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plain example:
---------------------------- STARTING LIVE TRADING ----------------------------
Waiting 1s for initial websockets snapshot.
Balance WS: { currency: '0.00000000',
asset: '0.04180751',
currency_hold: '0.00000000',
asset_hold: '0.00000000' }
Balance WS: { currency: '0.00000000',
asset: '0.04180751',
currency_hold: '0.00000000',
asset_hold: '0.00000000' }
2017-08-09 17:34:47 3308.40 BTC-USD +0.1% 6 61 ++ 0.0158 0.0099 0.04181 BTC 0.00000 USD +1.8% +0.0%
---------------------------- STARTING LIVE TRADING ----------------------------
Balance REST: { asset: '0.04180751',
currency: '0.00000000',
asset_hold: '0.03265206',
currency_hold: '0.00000000' }
Balance REST: { asset: '0.04180751',
currency: '0.00000000',
asset_hold: '0.03265206',
currency_hold: '0.00000000' }
2017-08-09 17:37:24 3310.00 BTC-USD +0.0% 13 63 ++ 0.0180 0.0092 0.04181 BTC 0.00000 USD +1.8% +0.0%
and this it not so tragic as this:
Should be like that:
2017-08-09 17:38:47 3310.10 BTC-USD +0.0% 6 63 ++ 0.0169 0.0091 0.04181 BTC 0.00000 USD +1.9% +0.0%
2017-08-09 17:38:52 - sell delayed: +78.1% of funds (0.03265206 BTC) on hold
Balance REST: { asset: '0.04180751',
currency: '0.00000000',
asset_hold: '0.03265206',
currency_hold: '0.00000000' }
Is like this:
2017-08-09 17:39:41 3310.10 BTC-USD +0.0% 26 63 ++ 0.0169 0.0091 0.04181 BTC 0.00000 USD +1.9% +0.0%
2017-08-09 17:39:58 - placing sell order...
2017-08-09 17:39:58 - sell order placed at 3310.10 USD
Balance WS: { currency: '0.00000000',
asset: '0.04180751',
currency_hold: '0.00000000',
asset_hold: '0.00000000' }
2017-08-09 17:40:03 - order status: rejected
2017-08-09 17:40:03 - not enough balance for sell, aborting
2017-08-09 17:40:03 - not enough balance, or signal switched, cancel sell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First let's correct balance, then test again.
One more problem! |
'b' or 'B' is not working at all in |
Manual mode (both buying and selling) is working fine for me. Just pushed a fix for zenbot commandline buy & sell. |
@@ -330,6 +343,9 @@ module.exports = function container (get, set, clear) { | |||
}, | |||
|
|||
trade: function (action, opts, cb) { | |||
if (!pair) { pair = joinProduct(opts.product_id) } | |||
var symbol = 't' + pair | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate in line 352
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a duplicate. The symbol on trade needs a 't' prefix, and in case the trade function is ever called as the first function, we need to know the pair and set it globally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please pay more attention:
trade: function (action, opts, cb) {
if (!pair) { pair = joinProduct(opts.product_id) }
var symbol = 't' + pair //line 347
client = authedClientWs();
var cid = Math.round(((new Date()).getTime()).toString() * Math.random())
var symbol = 't' + joinProduct(opts.product_id) //line 352
var amount = action === 'buy' ? opts.size : opts.size * -1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry, misunderstood your comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comments
…ts and retun updated balances
Calculation of |
Something is not right. Did you test?
|
I tested and am currently running live. Please run with
and look at the output. When restarting too quickly when testing you might get an error like "api key invalid", but after waiting some time it's back to normal. |
I even changed my API keys... and DEBUG show auth OK. But it just hangs on Waiting 1s for initial websockets snapshot. Waiting 1s for initial websockets snapshot. Waiting 1s for initial websockets snapshot. |
Reverted to previous and works again. I think all ws must be done outside getBalance. |
Reworked everything to use only one websockets connection, which is established very early. That should speed things up to jump into live trading. |
No luck:(
|
Hey,
I ported the bitfinex API to websockets. Backfilling is done using REST. Have been using it for about a week or more.
Also fixed postonly cancels to actually trigger re-buys ;)
Best regards,
Christian